Add RuntimeClassName feature flag by ianlewis · Pull Request #9072 · knative/serving

ianlewis · 2020-08-15T10:09:16Z

Allow users to set runtimeClassName on services via the runtimeclassname
feature flag.

Release Note

Added RuntimeClassName feature flag.

knative-prow-robot · 2020-08-15T10:09:29Z

Welcome @ianlewis! It looks like this is your first PR to knative/serving 🎉

knative-prow-robot · 2020-08-15T10:09:29Z

Hi @ianlewis. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

julz · 2020-08-17T07:01:01Z

pkg/apis/serving/fieldmask.go

 	if cfg.Features.PodSpecNodeSelector != config.Disabled {
 		out.NodeSelector = in.NodeSelector
 	}
+	if cfg.Features.PodSpecRuntimeClassName != config.Disabled {


worth adding a test for this, along same lines as the one for PodSpecTolerations?

julz · 2020-08-17T07:05:14Z

/ok-to-test

julz · 2020-08-17T07:09:58Z

@ianlewis out of interest, Im wondering whether using a PodSecurityPolicy to default RuntimeClass would fit your use case? Are there cases where you want the user to explicitly select runtimeClass for their ksvc, rather than having it implicit in a security policy (or an operator-set default)?

ianlewis

@ianlewis out of interest, Im wondering whether using a PodSecurityPolicy to default RuntimeClass would fit your use case? Are there cases where you want the user to explicitly select runtimeClass for their ksvc, rather than having it implicit in a security policy (or an operator-set default)?

Thanks. That's a good question. I think adding the feature flag is the best option for a few reasons:

I am trying to meet the needs of a broad set of customer use cases which may require sandboxed workloads to run alongside non-sandboxed workloads.
PodSecurityPolicy is in beta and has had some growing pains IIRC. I don't feel comfortable right now asking customers to rely on it.
Some customers will want to use PSP for validation rather than defaulting. i.e. set desired state explicitly themselves and validate using PSP, rather than setting the default for a namespace.
PSP can be used as an alternative to, or in addition to this feature flag. i.e. they don't interfere with each other so it provides more flexibility and covers more use cases.

ianlewis · 2020-08-19T00:39:37Z

pkg/apis/serving/fieldmask.go

 	if cfg.Features.PodSpecNodeSelector != config.Disabled {
 		out.NodeSelector = in.NodeSelector
 	}
+	if cfg.Features.PodSpecRuntimeClassName != config.Disabled {


knative-test-reporter-robot · 2020-08-19T01:01:12Z

The following jobs failed:

Test name	Triggers	Retries
pull-knative-serving-upgrade-tests	2020-08-19 01:01:10.785 +0000 UTC	1/3

Automatically retrying due to test flakiness...
/test pull-knative-serving-upgrade-tests

knative-test-reporter-robot · 2020-08-19T01:01:12Z

The following jobs failed:

Test name	Triggers	Retries
pull-knative-serving-integration-tests	2020-08-19 01:01:10.773 +0000 UTC	1/3

Automatically retrying due to test flakiness...
/test pull-knative-serving-integration-tests

knative-test-reporter-robot · 2020-08-19T01:01:12Z

The following jobs failed:

Test name	Triggers	Retries
pull-knative-serving-autotls-tests	2020-08-19 01:01:10.801 +0000 UTC	1/3

Automatically retrying due to test flakiness...
/test pull-knative-serving-autotls-tests

julz · 2020-08-19T10:57:41Z

/retest

julz

/lgtm

julz · 2020-08-20T07:16:03Z

/assign @dprotaso @mattmoor

Allow users to set runtimeClassName on services via the runtimeclassname feature flag. Issue knative#5306

knative-metrics-robot · 2020-08-24T00:39:49Z

The following is the coverage report on the affected files.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/fieldmask.go	98.1%	98.1%	0.0

mattmoor · 2020-08-27T02:09:43Z

pkg/apis/serving/k8s_validation_test.go

 }

 func TestPodSpecFeatureValidation(t *testing.T) {
+	runtimeClassName := "test"


I assume there is no good way to validate the values passed (besides podspec dry run?), since it varies so much vendor to vendor...

I suppose the controller could validate that the RuntimeClass with that name exists and add to the knative service's status and emit an event if it doesn't. Currently the controller will fail to create pods and users will see that the service didn't create and see the detailed reason in events already so I'm not sure it would be a much better UX.

mattmoor · 2020-08-27T02:19:00Z

/lgtm
/approve

knative-prow-robot · 2020-08-27T02:19:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ianlewis, mattmoor

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~config/OWNERS~~ [mattmoor]
~~pkg/apis/OWNERS~~ [mattmoor]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Aug 15, 2020

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 15, 2020

knative-prow-robot requested review from grantr and mdemirhan August 15, 2020 10:09

knative-prow-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. area/API API objects and controllers labels Aug 15, 2020

ianlewis mentioned this pull request Aug 15, 2020

knative workloads google/gvisor#3634

Closed

julz reviewed Aug 17, 2020

View reviewed changes

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 17, 2020

ianlewis force-pushed the runtimeclass branch from 39b311c to 0fda3dd Compare August 19, 2020 00:40

ianlewis commented Aug 19, 2020

View reviewed changes

julz reviewed Aug 19, 2020

View reviewed changes

knative-prow-robot assigned julz Aug 19, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 19, 2020

knative-prow-robot assigned dprotaso and mattmoor Aug 20, 2020

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 23, 2020

ianlewis added 2 commits August 23, 2020 20:33

Add RuntimeClassName feature flag

7e54b1c

Allow users to set runtimeClassName on services via the runtimeclassname feature flag. Issue knative#5306

Add validation test for RuntimeClassName

f969c94

ianlewis force-pushed the runtimeclass branch from 0fda3dd to f969c94 Compare August 24, 2020 00:33

knative-prow-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 24, 2020

mattmoor reviewed Aug 27, 2020

View reviewed changes

mattmoor mentioned this pull request Aug 27, 2020

Page documenting Serving feature flags knative/docs#2782

Closed

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 27, 2020

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 27, 2020

knative-prow-robot merged commit 6657049 into knative:master Aug 27, 2020

julz mentioned this pull request Apr 15, 2021

Add julz to API Approvers knative/community#573

Merged

clubanderson mentioned this pull request Mar 19, 2026

🌱 knative: Allow runtimeClassName and dnsPolicy in service template spec kubestellar/console-kb#1487

Merged

Conversation

ianlewis commented Aug 15, 2020

Uh oh!

knative-prow-robot commented Aug 15, 2020

Uh oh!

knative-prow-robot commented Aug 15, 2020

Uh oh!

julz Aug 17, 2020

Choose a reason for hiding this comment

Uh oh!

ianlewis Aug 19, 2020

Choose a reason for hiding this comment

Uh oh!

julz commented Aug 17, 2020

Uh oh!

julz commented Aug 17, 2020

Uh oh!

ianlewis left a comment

Choose a reason for hiding this comment

Uh oh!

ianlewis Aug 19, 2020

Choose a reason for hiding this comment

Uh oh!

knative-test-reporter-robot commented Aug 19, 2020

Uh oh!

knative-test-reporter-robot commented Aug 19, 2020

Uh oh!

knative-test-reporter-robot commented Aug 19, 2020

Uh oh!

julz commented Aug 19, 2020

Uh oh!

julz left a comment

Choose a reason for hiding this comment

Uh oh!

julz commented Aug 20, 2020

Uh oh!

knative-metrics-robot commented Aug 24, 2020

Uh oh!

mattmoor Aug 27, 2020

Choose a reason for hiding this comment

Uh oh!

ianlewis Aug 27, 2020

Choose a reason for hiding this comment

Uh oh!

mattmoor commented Aug 27, 2020

Uh oh!

knative-prow-robot commented Aug 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants